native build directories hardlinking file fetcher de-duplicates in-flight download requests #206

HongboDu-at · 2026-01-07T19:22:30Z

From internal usage, this reduces fetching inputs(~4GB) time from ~60s to ~20s.

…ight download requests

aspect-workflows · 2026-01-07T19:25:18Z

Test

Buildkite build #79 is running...

EdSchouten · 2026-01-07T21:00:27Z

pkg/cas/hardlinking_file_fetcher.go

+		select {
+		case <-d.wait:
+			if d.err != nil {
+				return d.err


This is incorrect. A caller of GetFile() may receive an error that was produced by another invocation. This should not happen, as errors may also include things like context cancelation, etc.

Updated to retry if it failed because of context cancelation

nit: bb_storage/pkg/auth/remoteAuthorizer.authorizeSingle also retries but has the same kind of flaw. Consider the case where the timeout is 55s, downloading the file takes 50s, one call is coming in every 30s and that the first call is cancelled after 35s. Then all retries for the later requests will also fail because when the retry starts, the context has less than 50s left.

0s RPC 0 30s RPC 1 45s RPC 0 cancelled, RPC 1 retries 60s RPC 2 85s RPC 1 times out (5s left to download), RPC 2 retries 90s RPC 3 115 RPC 2 times out (15s left to download), RPC 3 retries 120s RPC 4 115 RPC 3 times out (15s left to download), RPC 4 retries ...

One solution is to manually cancel a context.Background() when there are no requests left waiting. I don't know how complex the implementation and tests for this kind of solution will be or if this is just not worth bothering with.

Can't we just eliminate the error propagation logic entirely? Just change that downloads map to:

downloads map[string]<-chan struct{}

Only use that to wait an existing download to complete.

Thanks for the suggestion. Updated to use the new map.

pkg/cas/hardlinking_file_fetcher.go

…undant download

moroten · 2026-01-08T07:53:19Z

pkg/cas/hardlinking_file_fetcher.go

+
+type download struct {
+	wait chan struct{}
+	err  error


Suggested change

err error

// err is available only when wait has been closed.

err error

moroten · 2026-01-08T07:55:34Z

pkg/cas/hardlinking_file_fetcher.go

+}
+
+type download struct {
+	wait chan struct{}


nit: It is possible to be more strict here with wait <-chan struct{} and further down use

wait := make(chan struct{}) d = &download{wait: wait} close(wait)

Thanks for the suggestion.

moroten · 2026-01-08T08:02:02Z

pkg/cas/hardlinking_file_fetcher.go

+	d = &download{wait: make(chan struct{})}
+	ff.downloads[key] = d
+	ff.downloadsLock.Unlock()


ff.downloadsLock does not necessarily need to be locked when setting d.err as it is only to be read after d.wait has been closed. This means that defer can be used.

Suggested change

d = &download{wait: make(chan struct{})}

ff.downloads[key] = d

ff.downloadsLock.Unlock()

wait := make(chan struct{})

d = &download{wait: wait}

ff.downloads[key] = d

ff.downloadsLock.Unlock()

defer func() {

ff.downloadsLock.Lock()

delete(ff.downloads, key)

ff.downloadsLock.Unlock()

close(wait)

}()

Thanks for the suggestion. Updated to use defer. The code is cleaner.

moroten · 2026-01-08T08:06:17Z

pkg/cas/hardlinking_file_fetcher.go

+	wasMissing, err = ff.tryLinkFromCache(key, directory, name)
+	if err == nil {
+		// File appeared in cache, no download needed.
+		ff.downloadsLock.Lock()
+		delete(ff.downloads, key)
+		ff.downloadsLock.Unlock()
+		close(d.wait)
+		return nil
+	} else if !wasMissing {
+		// tryLinkFromCache had a real error (not just missing).
+		ff.downloadsLock.Lock()
+		d.err = err
+		delete(ff.downloads, key)
+		ff.downloadsLock.Unlock()
+		close(d.wait)
 		return err
 	}


See comment on line 125.

Suggested change

wasMissing, err = ff.tryLinkFromCache(key, directory, name)

if err == nil {

// File appeared in cache, no download needed.

ff.downloadsLock.Lock()

delete(ff.downloads, key)

ff.downloadsLock.Unlock()

close(d.wait)

return nil

} else if !wasMissing {

// tryLinkFromCache had a real error (not just missing).

ff.downloadsLock.Lock()

d.err = err

delete(ff.downloads, key)

ff.downloadsLock.Unlock()

close(d.wait)

return err

}

wasMissing, d.err = ff.tryLinkFromCache(key, directory, name)

if !wasMissing {

// Either the file appeared in cache, no download needed,

// or tryLinkFromCache had a real error (not just missing).

return d.err

}

moroten · 2026-01-08T08:36:03Z

pkg/cas/hardlinking_file_fetcher.go

+		select {
+		case <-d.wait:
+			if d.err != nil {
+				return d.err


nit: bb_storage/pkg/auth/remoteAuthorizer.authorizeSingle also retries but has the same kind of flaw. Consider the case where the timeout is 55s, downloading the file takes 50s, one call is coming in every 30s and that the first call is cancelled after 35s. Then all retries for the later requests will also fail because when the retry starts, the context has less than 50s left.

0s RPC 0 30s RPC 1 45s RPC 0 cancelled, RPC 1 retries 60s RPC 2 85s RPC 1 times out (5s left to download), RPC 2 retries 90s RPC 3 115 RPC 2 times out (15s left to download), RPC 3 retries 120s RPC 4 115 RPC 3 times out (15s left to download), RPC 4 retries ...

One solution is to manually cancel a context.Background() when there are no requests left waiting. I don't know how complex the implementation and tests for this kind of solution will be or if this is just not worth bothering with.

HongboDu-at

Thank you for the review.

HongboDu-at · 2026-01-09T00:39:47Z

pkg/cas/hardlinking_file_fetcher.go

+}
+
+type download struct {
+	wait chan struct{}


Thanks for the suggestion.

HongboDu-at · 2026-01-09T00:41:31Z

pkg/cas/hardlinking_file_fetcher.go

+		select {
+		case <-d.wait:
+			if d.err != nil {
+				return d.err


Thanks for the suggestion. Updated to use the new map.

HongboDu-at · 2026-01-09T00:41:55Z

pkg/cas/hardlinking_file_fetcher.go

+	d = &download{wait: make(chan struct{})}
+	ff.downloads[key] = d
+	ff.downloadsLock.Unlock()


Thanks for the suggestion. Updated to use defer. The code is cleaner.

EdSchouten

Thanks for working on this!

EdSchouten · 2026-01-11T15:59:11Z

pkg/cas/hardlinking_file_fetcher.go

+			if err == nil {
+				return nil
+			} else if wasMissing {
+				return ff.GetFile(ctx, blobDigest, directory, name, isExecutable)


We're calling GetFile() recursively here. Though I get why you're doing this, I think it hurts readability. Can't we add a for { ... } to the top level of GetFile() instead?

Thanks for the suggestion. Changed to add a for { ... } to the top level of GetFile() instead

EdSchouten · 2026-01-11T16:01:35Z

pkg/cas/hardlinking_file_fetcher.go

+	_, ok := ff.filesSize[key]
+	ff.filesLock.RUnlock()
+	if !ok {
+		return true, os.ErrNotExist


Is this wasMissingreturn value really needed? Wouldn't it be sufficient to just call os.IsNotExist(err) at the call site?

Thanks for the suggestion. Changed to just call os.IsNotExist(err)

EdSchouten

I think this change is starting to shape up nicely. I have one remaining comment. Be sure to address this, and I'll try to merge this quickly. Thanks!

EdSchouten · 2026-01-13T10:43:08Z

pkg/cas/hardlinking_file_fetcher.go

+	if downloadErr == nil {
+		// The file was downloaded successfully. Place it into the
+		// cache, so that successive calls may use it.
+		ff.filesLock.Lock()
+		if _, ok := ff.filesSize[key]; !ok {
+			ff.evictionLock.Lock()
+
+			// Remove old files from the cache if necessary.
+			sizeBytes := blobDigest.GetSizeBytes()
+			if err := ff.makeSpace(sizeBytes); err != nil {
+				downloadErr = err
+			} else {
+				// Hardlink the file into the cache.
+				if err := directory.Link(name, ff.cacheDirectory, path.MustNewComponent(key)); err != nil && !os.IsExist(err) {
+					downloadErr = util.StatusWrapfWithCode(err, codes.Internal, "Failed to add cached file %#v", key)
+				} else {
+					ff.evictionSet.Insert(key)
+					ff.filesSize[key] = sizeBytes
+					ff.filesTotalSize += sizeBytes
+				}
+			}
+			ff.evictionLock.Unlock()
+		} else {
+			// The file was already part of our bookkeeping,
+			// but was missing on disk. Repair this by adding
+			// a link to the newly downloaded file.
+			if err := directory.Link(name, ff.cacheDirectory, path.MustNewComponent(key)); err != nil && !os.IsExist(err) {
+				downloadErr = util.StatusWrapfWithCode(err, codes.Internal, "Failed to repair cached file %#v", key)
+			}
 		}
+		ff.filesLock.Unlock()
+	}

-		// Hardlink the file into the cache.
-		if err := directory.Link(name, ff.cacheDirectory, path.MustNewComponent(key)); err != nil && !os.IsExist(err) {
-			return util.StatusWrapfWithCode(err, codes.Internal, "Failed to add cached file %#v", key)
-		}
-		ff.evictionSet.Insert(key)
-		ff.filesSize[key] = sizeBytes
-		ff.filesTotalSize += sizeBytes
-	} else if wasMissing {
-		// Even though the file is part of our bookkeeping, we
-		// observed it didn't exist. Repair this inconsistency.
-		if err := directory.Link(name, ff.cacheDirectory, path.MustNewComponent(key)); err != nil && !os.IsExist(err) {
-			return util.StatusWrapfWithCode(err, codes.Internal, "Failed to repair cached file %#v", key)
-		}
+	return downloadErr


I'm just trying to figure out why this change is still needed. It probably isn't...? If so, can we revert this part?

Thanks for the suggestion. Reverted the this cache insertion part.

The eviction set must always remain in sync with the map tracked by the hardlinking file fetcher. This is why any mutations made to it were covered by the read lock. In #206 it looks like this code was slightly shuffled around, causing both to be locked independently. Fixes: #207

native build directories hardlinking file fetcher de-duplicates in-fl…

44ac449

…ight download requests

EdSchouten reviewed Jan 7, 2026

View reviewed changes

Callers retry download for context cancellation; fix one possible red…

5c7e965

…undant download

HongboDu-at requested a review from EdSchouten January 7, 2026 23:13

moroten reviewed Jan 8, 2026

View reviewed changes

use downloads map[string]<-chan struct{} and defer

cc74acf

HongboDu-at commented Jan 9, 2026

View reviewed changes

EdSchouten reviewed Jan 11, 2026

View reviewed changes

call call os.IsNotExist(err); For loop GetFile()

e12099a

HongboDu-at requested a review from EdSchouten January 12, 2026 21:50

EdSchouten reviewed Jan 13, 2026

View reviewed changes

revert the cache insertion part

deb36df

EdSchouten approved these changes Jan 13, 2026

View reviewed changes

EdSchouten merged commit 76477ed into buildbarn:main Jan 14, 2026
3 checks passed

EdSchouten mentioned this pull request Jan 19, 2026

Crash of bb_worker on AppleSilicon Mac #207

Closed

	err error
	// err is available only when wait has been closed.
	err error

native build directories hardlinking file fetcher de-duplicates in-flight download requests #206

native build directories hardlinking file fetcher de-duplicates in-flight download requests #206

Conversation

HongboDu-at commented Jan 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aspect-workflows bot commented Jan 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HongboDu-at left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EdSchouten left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EdSchouten left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

HongboDu-at commented Jan 7, 2026 •

edited

Loading

aspect-workflows bot commented Jan 7, 2026 •

edited

Loading